Livecheck#url: Don't convert URL symbol to string #10309
Conversation
|
Review period will end on 2021-01-14 at 03:14:54 UTC. |
BrewTestBot
added
the
waiting for feedback
| HEAD_URL = "https://github.com/Homebrew/brew.git" | ||
| HOMEPAGE_URL = "https://brew.sh" | ||
| LIVECHECK_URL = "https://formulae.brew.sh/api/formula/ruby.json" | ||
| STABLE_URL = "https://brew.sh/test-0.0.1.tgz" |
There was a problem hiding this comment.
Use let instead of constants.
There was a problem hiding this comment.
I agree that let is more appropriate. I was using constants because a string that's defined using let isn't available in the formula and cask blocks. When using a constant, url STABLE_URL works in the f formula. When using let(:stable_url) { ... }, url stable_url gives an undefined local variable or method `stable_url' error.
My goals were to reduce repetition of string literals in multiple places and to make the tests easier to read (i.e., it's somewhat ambiguous as to what "https://brew.sh/test-0.0.1.tgz" references but STABLE_URL is clear).
I've updated this to use string literals in the formula/cask definitions and let to define strings that can be referenced in tests (so we can use something that's readily understandable like stable_url instead of an ambiguous string literal).
I'm considering trying to extract the various formulae/casks that we define in livecheck's tests into separate files (like test/support/fixtures/cask/Casks) sometime in the future. If we go down that path, we would have to lose the constants anyway, so I'm good with the let approach here.
BrewTestBot
removed
the
waiting for feedback
|
Review period ended. |
brew stylewith your changes locally?brew typecheckwith your changes locally?brew testswith your changes locally?brew manlocally and committed any changes?An issue that we've encountered in at least one homebrew/core formula is that
url :headwon't work in alivecheckblock if the formula uses aheadblock. Thelivecheckblock comes before theheadblock, so theheadURL isn't defined when thelivecheckblock#urlmethod is called.In #10157, I proposed that we could resolve this issue by moving
headblocks afterstableblocks in formulae, so they both come before thelivecheckblock. Some Homebrew members supported this while others preferred to simply move thelivecheckblock after theheadblock. Both of these options would lead to some level of unhappiness in the end.While discussing these options, @SeekingMeaning asked if we could modify
Livecheck#urlto simply store the symbol from thelivecheckblock and convert it to the formula URL string sometime after the formula is parsed. I had already considered doing this for other reasons (see #10157 (comment) for details), so I agreed that this was the most amenable solution to this particular issue.With that in mind, this PR modifies
Livecheck#urlto simply store the value it's passed (a string or symbol). We do the conversion from a URL symbol to the formula/cask URL string in#latest_version, using a new#livecheck_url_to_stringmethod.This solution resolves the issue described above but it also allows us to surface information in the debug and JSON output about whether a URL symbol was used in the
livecheckblock. The existingLivecheck#urlsetup prevented us from being able to do this before and that was initially why I thought about pursuing this change.Along the way, I did some minor refactoring in livecheck's tests. There's still a lot of room for improvement when it comes to tidying up these tests but that's something for a later PR.
If/when this is merged, I'll close #10157 and Homebrew/homebrew-core#67824, as they won't be necessary.